Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Send zero-length NST when session key is expired #4532

Merged
merged 36 commits into from
May 15, 2024

Conversation

jouho
Copy link
Contributor

@jouho jouho commented Apr 30, 2024

Resolved issues:

Resolves #4528

Description of changes:

According to RFC5077:

This message MUST be sent if the
server included a SessionTicket extension in the ServerHello.
...
If the server determines that it does not want to include a
ticket after it has included the SessionTicket extension in the
ServerHello, then it sends a zero-length ticket in the
NewSessionTicket handshake message.

Currently we throw an error during the write of NewSessionTicket message if there are no valid session ticket keys available. It is guarded by s2n_encrypt_session_ticket, which fails if there is no key available. This behavior is verified and can be reproduced by running the new test added with this change reverted.

This change modifies the condition of s2n_server_nst_send in s2n-tls/tls/s2n_server_new_session_ticket.c so that it writes a zero-length session ticket if the key used to encrypt session ticket was expired between SERVER_HELLO and CLIENT_FINISHED, before writing NewSessionTicket.

This change does not impact TLS1.3, as it bail and don't send it if no key is available: https://github.com/aws/s2n-tls/blob/main/tls/s2n_server_new_session_ticket.c#L170-L172. A test to verify this is also added.

Does this change what S2N sends over the wire?
It will now correctly sends a zero-length NewSessionTicket message in the condition described above.
Does this change any public APIs? If yes, explain.
No.
Which versions of TLS will this impact?
This will impact TLS1.2 and not TLS1.3.

Call-outs:

Currently I am verifying that client received zero-length session ticket by
EXPECT_EQUAL(client_connection->client_ticket.allocated, 0);, but this may not explicitly verify the condition and there could be a better way to test it.

Testing:

Confirmed all unit tests pass.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Apr 30, 2024
@jouho jouho requested review from lrstewart and maddeleine May 1, 2024 00:32
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
@jouho jouho requested a review from maddeleine May 6, 2024 22:20
tls/s2n_server_new_session_ticket.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_server_new_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_server_new_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_server_new_session_ticket_test.c Outdated Show resolved Hide resolved
@jouho jouho requested a review from maddeleine May 8, 2024 20:21
tests/unit/s2n_server_new_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_server_new_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_server_new_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_server_new_session_ticket_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_session_ticket_test.c Show resolved Hide resolved
@jouho jouho requested a review from maddeleine May 8, 2024 22:45
tls/s2n_server_new_session_ticket.c Outdated Show resolved Hide resolved
tls/s2n_server_new_session_ticket.c Outdated Show resolved Hide resolved
tls/s2n_server_new_session_ticket.c Outdated Show resolved Hide resolved
@jouho jouho requested a review from lrstewart May 13, 2024 20:18
@jouho jouho requested a review from lrstewart May 15, 2024 19:27
Co-authored-by: Lindsay Stewart <[email protected]>
@jouho jouho enabled auto-merge (squash) May 15, 2024 19:40
@jouho jouho merged commit 885b607 into main May 15, 2024
36 checks passed
@jouho jouho deleted the zero-length-tls12-NST-message branch May 15, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send zero-length TLS1.2 NewSessionTicket message if necessary
3 participants